Skip to content

Introduce coerce_container_to_any #14812

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Introduce coerce_container_to_any #14812

wants to merge 1 commit into from

Conversation

Ralith
Copy link

@Ralith Ralith commented May 15, 2025 •
edited
Loading

This PR introduces a lint which detects when &Box<dyn Any> is coerced to &dyn Any, which is usually a mistake where the intent was to directly pass the dyn Any inside the box without coercion.

changelog: [coerce_container_to_any]: introduce lint

cc @llogiq

Remaining work:

  • Documentation
  • Generalize from Box to any implementer of Deref<Target=dyn Any>

@rustbot
Copy link
Collaborator

rustbot commented May 15, 2025

r? @dswij

rustbot has assigned @dswij.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 15, 2025
@matthiasbeyer
Copy link

r? llogiq

@rustbot rustbot assigned llogiq and unassigned dswij May 15, 2025
@llogiq
Copy link
Contributor

llogiq commented May 16, 2025

This looks good for a start. Let's add the documentation and replace the "default lint description", and then I can start the Final Comment Period for this lint.

@Ralith Ralith force-pushed the coerce_any_ref_to_any branch from 9950e08 to 2363e9d Compare May 17, 2025 13:23
@Ralith Ralith changed the title WIP: Introduce coerce_any_ref_to_any Introduce coerce_any_ref_to_any May 17, 2025
@Ralith
Copy link
Author

Ralith commented May 17, 2025

Thanks to guidance from @y21 on Zulip, I've got this as smart as I wanted. Seeking feedback particularly on lint name and description wording. I may iterate on the diagnostic a little more.

@Ralith Ralith force-pushed the coerce_any_ref_to_any branch from 2363e9d to 18d86b3 Compare May 17, 2025 13:53
@Ralith
Copy link
Author

Ralith commented May 17, 2025

Diagnostic is now pretty good. Not sure if there's a better way to format the expression type, e.g. so it shows up as Box rather than std::boxed::Box.

@Ralith Ralith force-pushed the coerce_any_ref_to_any branch 11 times, most recently from 4438e38 to 037945c Compare May 17, 2025 17:40
@Ralith
Copy link
Author

Ralith commented May 17, 2025

Future work: maybe this should also fire on coercing &Box<dyn Foo> to &dyn Any when Foo: Any?

@rustbot

This comment has been minimized.

Sign in to view
@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 17, 2025
@Ralith Ralith force-pushed the coerce_any_ref_to_any branch from fa326e3 to 71bc081 Compare May 17, 2025 19:06
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 17, 2025
@Ralith
Copy link
Author

Ralith commented May 17, 2025

Thanks to @fu5ha for expanded docs!

@Ralith Ralith force-pushed the coerce_any_ref_to_any branch from 71bc081 to eef8a8c Compare May 17, 2025 19:35
sanbox-irl
sanbox-irl reviewed May 18, 2025
View reviewed changes
Copy link

@sanbox-irl sanbox-irl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wonderful! I left some bikeshedding comments on the description and what not.

For the name, what about coerce_container_to_any? I think using any_ref to describe &Box<dyn Any> is surprising

@llogiq
Copy link
Contributor

llogiq commented May 18, 2025

Not sure if there's a better way to format the expression type, e.g. so it shows up as Box rather than std::boxed::Box.

That would be path trimming, which we currently don't have. I'm on that, but you don't need to worry about it right now.

@Ralith Ralith force-pushed the coerce_any_ref_to_any branch from eef8a8c to 27eec99 Compare May 19, 2025 01:51
@Ralith
Copy link
Author

Ralith commented May 19, 2025

Applied all feedback. Thanks for the reviews!

@Ralith Ralith changed the title Introduce coerce_any_ref_to_any Introduce coerce_container_to_any May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@llogiq llogiq llogiq left review comments

@jackh726 jackh726 jackh726 left review comments

@sanbox-irl sanbox-irl sanbox-irl left review comments

Assignees

@llogiq llogiq

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Ralith @rustbot @matthiasbeyer @llogiq @jackh726 @sanbox-irl @dswij